Skip to content

Prefer destroy over delete volume for expunging - #349

Open
bddvlpr wants to merge 1 commit into
apache:mainfrom
bddvlpr:fix/delete-volume
Open

bddvlpr wants to merge 1 commit into
apache:mainfrom
bddvlpr:fix/delete-volume

Conversation

@bddvlpr

@bddvlpr bddvlpr commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Adapting the disk resource according to the instance standard as it also uses destroy. This destroys the disk but (optionally) leaves it unexpunged for manual cleanup or satefy measures.

Acceptance tests pass, also tested on an actual setup.

Copilot AI lite review requested due to automatic review settings September 14, 2026 12:07

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Add acceptance coverage verifying that unset or false expunge leaves the destroyed volume available for manual cleanup.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Updates cloudstack_disk to use CloudStack’s destroy API with optional volume expunging, consistent with instance behavior.

Changes:

  • Adds the expunge argument, defaulting to false.
  • Replaces DeleteVolume with conditional destruction and expunging.
  • Updates documentation and acceptance configurations.
File summaries
File Summary
website/docs/r/disk.html.markdown Documents the expunge argument.
cloudstack/resource_cloudstack_disk.go Implements configurable volume destruction.
cloudstack/resource_cloudstack_disk_test.go Updates tests; a nit requests coverage for the default expunge = false behavior.
Review details
  • Files reviewed: 3/3 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread cloudstack/resource_cloudstack_disk_test.go
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants